fix(classifier): avoid forcing unsupported temperature - #2
Conversation
1bc95a4 to
d425d00
Compare
There was a problem hiding this comment.
Thanks for the PR, good find! Nonetheless, I have to ask for a few changes.
-
P1 — fail closed based on
stopReason, not the error text (extensions/auto-mode/classifier.ts:177-200): an error response witherrorMessage: ""is treated as non-error. If it also contains valid allow JSON,classifyWithRetryreturnsallow. I reproduced that case. Branch onresponse.stopReason === "error"and useresponse.errorMessage || <fallback>only for the displayed reason. Add a regression test for the empty-message case. -
P1 — rebase and resolve the conflicts with current
main: this branch conflicts with the ccusage observability work inclassifier.ts,types.ts, tests, anddocs/observability-logging.md. Preserve current main'smodel,timestamp, andusagefields while addingerrorMessage?; taking either side wholesale loses useful observability data. -
P3 — strengthen the temperature regression test (
tests/auto-mode.test.ts:396-424): reading.temperaturedoes not distinguish an omitted property from{ temperature: undefined }. AssertObject.hasOwn(callOptions, "temperature") === falsefor the default path.
PR-head validation passed: npm run check, 65 tests, npm pack --dry-run, and git diff --check. The post-conflict merged result still needs the same validation.
Let classifier providers use their default temperature unless a caller explicitly supplies one. Some providers, including OpenAI Codex, reject the parameter entirely. Treat provider-reported completion errors as classifier failures instead of retrying them as malformed JSON, and retain the provider message in classifier I/O logs.
d425d00 to
6d73d98
Compare
|
@czottmann Done ✔️ |
|
Thanks, @tank-bohr! Had to manually merge your contributions as |
2 participants
Problem
Classifier calls currently force
temperature: 0. Providers that do not support the parameter, including OpenAI Codex, reject the request before classification:pi-airepresents this as an assistant response withstopReason: "error". The classifier then treats the empty response body as malformed JSON, retries the same request, and reports a misleading invalid-JSON failure.Changes
Verification
npm run checknpm test(65 tests)npm pack --dry-runopenai-codex/gpt-5.6-solwith no temperature override returned a valid allow decision.